Skip to content

StreamStringTermsAggregator explicitly asserts single leaf #19416

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Oct 7, 2025
Merged

StreamStringTermsAggregator explicitly asserts single leaf #19416

merged 9 commits into from
Oct 7, 2025

Conversation

@mkhludnev
Copy link
Contributor

@mkhludnev mkhludnev commented Sep 25, 2025

Continuation of #19404.

@mkhludnev mkhludnev mentioned this pull request Sep 25, 2025
Merged
3 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Sep 25, 2025

❌ Gradle check result for f95c056: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@opensearch-ci-bot opensearch-ci-bot mentioned this pull request Sep 25, 2025
Closed
@mkhludnev mkhludnev force-pushed the stream-agg-assert-single-list branch from f95c056 to 9b1eb64 Compare September 25, 2025 18:05
@github-actions
Copy link
Contributor

github-actions bot commented Sep 25, 2025

✅ Gradle check result for 9b1eb64: SUCCESS

@codecov
Copy link

codecov bot commented Sep 25, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.95%. Comparing base (ac6dfa1) to head (3969e52).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main   #19416      +/-   ##
============================================
- Coverage     73.09%   72.95%   -0.14%     
+ Complexity    70553    70424     -129     
============================================
  Files          5716     5716              
  Lines        322926   322935       +9     
  Branches      46770    46770              
============================================
- Hits         236032   235595     -437     
- Misses        67882    68363     +481     
+ Partials      19012    18977      -35

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mkhludnev
set flush mode per segment
6347a4a 
Signed-off-by: Mikhail Khludnev <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2025

❌ Gradle check result for 6347a4a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@mkhludnev
Copy link
Contributor Author

mkhludnev commented Oct 4, 2025 •
edited
Loading

aghh... adding assert into StreamSTAgg.getLeafCollector() reveals the second invocation by ProfilingAggregator. It causes the failure in SubAggregationIT/testStreamingAggregationUsed. It needs to be covered with this unit test.

at org.opensearch.search.aggregations.bucket.terms.StreamStringTermsAggregator.getLeafCollector(StreamStringTermsAggregator.java:94)
	at org.opensearch.search.aggregations.AggregatorBase.getLeafCollector(AggregatorBase.java:210)
	at org.opensearch.search.profile.aggregation.ProfilingAggregator.getLeafCollector(ProfilingAggregator.java:126)
	at org.opensearch.search.profile.aggregation.ProfilingAggregator.getLeafCollector(ProfilingAggregator.java:53)
	at org.apache.lucene.search.FilterCollector.getLeafCollector(FilterCollector.java:38)
	at org.opensearch.search.profile.query.ProfileCollector.getLeafCollector(ProfileCollector.java:85)
	at org.opensearch.search.profile.query.InternalProfileCollector.getLeafCollector(InternalProfileCollector.java:147)
	at org.apache.lucene.search.MultiCollector.getLeafCollector(MultiCollector.java:130)
	at org.apache.lucene.search.FilterCollector.getLeafCollector(FilterCollector.java:38)
	at org.opensearch.search.profile.query.ProfileCollector.getLeafCollector(ProfileCollector.java:85)
	at org.opensearch.search.profile.query.InternalProfileCollector.getLeafCollector(InternalProfileCollector.java:147)
	at org.opensearch.search.internal.ContextIndexSearcher.searchLeaf(ContextIndexSearcher.java:351)
	at org.opensearch.search.internal.ContextIndexSearcher.search(ContextIndexSearcher.java:316)
	at org.opensearch.search.internal.ContextIndexSearcher.search(ContextIndexSearcher.java:280)
	at org.opensearch.search.query.QueryPhase.searchWithCollector(QueryPhase.java:356)
	at org.opensearch.search.query.QueryPhase$DefaultQueryPhaseSearcher.searchWithCollector(QueryPhase.java:493)
	at org.opensearch.search.query.QueryPhase$DefaultQueryPhaseSearcher.searchWithCollector(QueryPhase.java:450)
	at org.opensearch.search.query.QueryPhase$DefaultQueryPhaseSearcher.searchWith(QueryPhase.java:433)
	at org.opensearch.search.query.QueryPhaseSearcherWrapper.searchWith(QueryPhaseSearcherWrapper.java:60)
	at org.opensearch.search.query.QueryPhase.executeInternal(QueryPhase.java:283)
	at org.opensearch.search.query.QueryPhase.execute(QueryPhase.java:156)
	at org.opensearch.search.SearchService.loadOrExecuteQueryPhase(SearchService.java:733)

@mkhludnev mkhludnev marked this pull request as draft October 4, 2025 17:27
@mkhludnev mkhludnev marked this pull request as ready for review October 5, 2025 20:30
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2025

❕ Gradle check result for 2771ebc: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@mkhludnev mkhludnev force-pushed the stream-agg-assert-single-list branch from 2771ebc to 0f84b06 Compare October 6, 2025 05:06
@mkhludnev
add CHANGELOG.md
3c0350f 
Signed-off-by: m.khludnev <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2025

❌ Gradle check result for 3c0350f: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@mkhludnev
Copy link
Contributor Author

mkhludnev commented Oct 6, 2025

Checked https://build.ci.opensearch.org/job/gradle-check/65053/ both seems unrelated.
@andrross I've decided to add changelog for this change after all.

@mkhludnev
join CHANGELOG.md line
3969e52 
Signed-off-by: m.khludnev <[email protected]>
@rishabhmaurya
Copy link
Contributor

rishabhmaurya commented Oct 6, 2025

There are 2 more places where we can add this check - https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/search/aggregations/bucket/terms/StreamNumericTermsAggregator.java, https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/search/aggregations/metrics/StreamCardinalityAggregator.java

@mkhludnev
Copy link
Contributor Author

mkhludnev commented Oct 6, 2025 •
edited
Loading

2 more places

I propose to address them in the separate PR, where we can introduce a base class enforcing reset() after a leaf is collected.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2025

✅ Gradle check result for 3c0350f: SUCCESS

@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2025

✅ Gradle check result for 3969e52: SUCCESS

@rishabhmaurya rishabhmaurya merged commit 2e1bb7c into opensearch-project:main Oct 7, 2025
33 checks passed
@rishabhmaurya
Copy link
Contributor

rishabhmaurya commented Oct 7, 2025

thank you @mkhludnev

This was referenced Oct 9, 2025
Open
Open
rgsriram pushed a commit to rgsriram/OpenSearch that referenced this pull request Oct 11, 2025
@mkhludnev @rgsriram
StreamStringTermsAggregator explicitly asserts single leaf (opensearc…
e28b0db 
…h-project#19416)

* Asserting single segment in StreamStringTermsAggregator

Signed-off-by: Mikhail Khludnev <[email protected]>

* Asserting single segment in StreamStringTermsAggregator

Dropping two RandomIndexWriters still cause flakiness ;( (before).

Signed-off-by: Mikhail Khludnev <[email protected]>

* adding test collecting per-segment aggs and reducing them to get overal count and values.

Signed-off-by: Mikhail Khludnev <[email protected]>

* set flush mode per segment

Signed-off-by: Mikhail Khludnev <[email protected]>

* propagate reset() through profiler.

Signed-off-by: m.khludnev <[email protected]>

* add CHANGELOG.md

Signed-off-by: m.khludnev <[email protected]>

* join CHANGELOG.md line

Signed-off-by: m.khludnev <[email protected]>

---------

Signed-off-by: Mikhail Khludnev <[email protected]>
Signed-off-by: Mikhail Khludnev <[email protected]>
Signed-off-by: m.khludnev <[email protected]>
Gagan6164 pushed a commit to Gagan6164/OpenSearch that referenced this pull request Oct 13, 2025
@mkhludnev
StreamStringTermsAggregator explicitly asserts single leaf (opensearc…
070b815 
…h-project#19416)

* Asserting single segment in StreamStringTermsAggregator

Signed-off-by: Mikhail Khludnev <[email protected]>

* Asserting single segment in StreamStringTermsAggregator

Dropping two RandomIndexWriters still cause flakiness ;( (before).

Signed-off-by: Mikhail Khludnev <[email protected]>

* adding test collecting per-segment aggs and reducing them to get overal count and values.

Signed-off-by: Mikhail Khludnev <[email protected]>

* set flush mode per segment

Signed-off-by: Mikhail Khludnev <[email protected]>

* propagate reset() through profiler.

Signed-off-by: m.khludnev <[email protected]>

* add CHANGELOG.md

Signed-off-by: m.khludnev <[email protected]>

* join CHANGELOG.md line

Signed-off-by: m.khludnev <[email protected]>

---------

Signed-off-by: Mikhail Khludnev <[email protected]>
Signed-off-by: Mikhail Khludnev <[email protected]>
Signed-off-by: m.khludnev <[email protected]>
Signed-off-by: Gagan Singh Saini <[email protected]>
peteralfonsi pushed a commit to peteralfonsi/OpenSearch that referenced this pull request Oct 15, 2025
@mkhludnev
StreamStringTermsAggregator explicitly asserts single leaf (opensearc…
a8f6219 
…h-project#19416)

* Asserting single segment in StreamStringTermsAggregator

Signed-off-by: Mikhail Khludnev <[email protected]>

* Asserting single segment in StreamStringTermsAggregator

Dropping two RandomIndexWriters still cause flakiness ;( (before).

Signed-off-by: Mikhail Khludnev <[email protected]>

* adding test collecting per-segment aggs and reducing them to get overal count and values.

Signed-off-by: Mikhail Khludnev <[email protected]>

* set flush mode per segment

Signed-off-by: Mikhail Khludnev <[email protected]>

* propagate reset() through profiler.

Signed-off-by: m.khludnev <[email protected]>

* add CHANGELOG.md

Signed-off-by: m.khludnev <[email protected]>

* join CHANGELOG.md line

Signed-off-by: m.khludnev <[email protected]>

---------

Signed-off-by: Mikhail Khludnev <[email protected]>
Signed-off-by: Mikhail Khludnev <[email protected]>
Signed-off-by: m.khludnev <[email protected]>
This was referenced Oct 22, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rishabhmaurya rishabhmaurya rishabhmaurya approved these changes

@anasalkouz anasalkouz Awaiting requested review from anasalkouz anasalkouz is a code owner

@andrross andrross Awaiting requested review from andrross andrross is a code owner

@ashking94 ashking94 Awaiting requested review from ashking94 ashking94 is a code owner

@Bukhtawar Bukhtawar Awaiting requested review from Bukhtawar Bukhtawar is a code owner

@CEHENKLE CEHENKLE Awaiting requested review from CEHENKLE CEHENKLE is a code owner

@cwperks cwperks Awaiting requested review from cwperks cwperks is a code owner

@dbwiddis dbwiddis Awaiting requested review from dbwiddis dbwiddis is a code owner

@gbbafna gbbafna Awaiting requested review from gbbafna gbbafna is a code owner

@jed326 jed326 Awaiting requested review from jed326 jed326 is a code owner

@kotwanikunal kotwanikunal Awaiting requested review from kotwanikunal kotwanikunal is a code owner

@mch2 mch2 Awaiting requested review from mch2 mch2 is a code owner

@msfroh msfroh Awaiting requested review from msfroh msfroh is a code owner

@owaiskazi19 owaiskazi19 Awaiting requested review from owaiskazi19 owaiskazi19 is a code owner

@reta reta Awaiting requested review from reta reta is a code owner

@Rishikesh1159 Rishikesh1159 Awaiting requested review from Rishikesh1159 Rishikesh1159 is a code owner

@sachinpkale sachinpkale Awaiting requested review from sachinpkale sachinpkale is a code owner

@saratvemulapalli saratvemulapalli Awaiting requested review from saratvemulapalli saratvemulapalli is a code owner

@shwetathareja shwetathareja Awaiting requested review from shwetathareja shwetathareja is a code owner

@sohami sohami Awaiting requested review from sohami sohami is a code owner

@bowenlan-amzn bowenlan-amzn Awaiting requested review from bowenlan-amzn

Assignees

No one assigned

Labels

skip-changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mkhludnev @andrross @rishabhmaurya @bowenlan-amzn